Updated pod response, grouped the parameters: "startip, endip, vlanid, forsystemvms" as ip range response and added to ipranges parameter. by sureshanaparti · Pull Request #5424 · apache/cloudstack

sureshanaparti · 2021-09-09T08:48:07Z

Description

This PR updates the pod response, grouped the parameters: startip, endip, vlanid, forsystemvms as ip range response and added to ipranges parameter (a new parameter to hold the list of IP range details).

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Manually tested the updated API response.

Before changes:

(cloudcmk) > list pods
{
  "count": 1,
  "pod": [
    {
      "allocationstate": "Enabled",
      "endip": [
        "10.0.36.60",
        "10.0.36.69"
      ],
      "forsystemvms": [
        "0",
        "1"
      ],
      "gateway": "10.0.32.1",
      "hasannotations": false,
      "id": "43ffd982-8942-4c9a-88f3-002dc7f5d411",
      "name": "Pod1",
      "netmask": "255.255.240.0",
      "startip": [
        "10.0.36.41",
        "10.0.36.67"
      ],
      "vlanid": [
        "vlan://untagged",
        "vlan://untagged"
      ],
      "zoneid": "be8e7d3e-2f57-433d-9c93-208949e2ec1d",
      "zonename": "ref-trl-1773-k-M7-suresh-anaparti"
    }
  ]
}

After changes: New parameter ipranges holds the list of ip ranges. Old parameters would be deprecated (kept for backward compatibility).

(cloudcmk) > list pods
{
  "count": 1,
  "pod": [
    {
      "allocationstate": "Enabled",
      "endip": [
        "10.0.36.60",
        "10.0.36.69"
      ],
      "forsystemvms": [
        "0",
        "1"
      ],
      "gateway": "10.0.32.1",
      "hasannotations": false,
      "id": "43ffd982-8942-4c9a-88f3-002dc7f5d411",
      "ipranges": [
        {
          "endip": "10.0.36.60",
          "forsystemvms": "0",
          "startip": "10.0.36.41",
          "vlanid": "vlan://untagged"
        },
        {
          "endip": "10.0.36.69",
          "forsystemvms": "1",
          "startip": "10.0.36.67",
          "vlanid": "vlan://untagged"
        }
      ],
      "name": "Pod1",
      "netmask": "255.255.240.0",
      "startip": [
        "10.0.36.41",
        "10.0.36.67"
      ],
      "vlanid": [
        "vlan://untagged",
        "vlan://untagged"
      ],
      "zoneid": "be8e7d3e-2f57-433d-9c93-208949e2ec1d",
      "zonename": "ref-trl-1773-k-M7-suresh-anaparti"
    }
  ]
}

sureshanaparti · 2021-09-09T08:51:36Z

@blueorangutan package

blueorangutan · 2021-09-09T08:52:03Z

@sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

shwstppr

I'm not sure if this helps to address the original issue (API automation breaking). Moreover, this further complicates it by again changing the listPods API response.
In my opinion, we should also update the description for the startip and endip params of the response, stating that they will return lists (and not a single IP)

sureshanaparti · 2021-09-09T09:02:34Z

I'm not sure if this helps to address the original issue. Moreover, this further complicates it by again changing the listPods API response.
In my opinion, we can just update the description for the startip and endip params of the response, stating that they will return lists (and not a single IP)

@shwstppr I think, It is better to keep the parameters "startip, endip, vlanid, forsystemvms" as a group, instead maintaining as separate lists and relating these by the list index.

sureshanaparti · 2021-09-09T09:09:10Z

I'm not sure if this helps to address the original issue. Moreover, this further complicates it by again changing the listPods API response.
In my opinion, we can just update the description for the startip and endip params of the response, stating that they will return lists (and not a single IP)

The original issue doesn't make sense as the Pod can hold multiple ip ranges, and I'm (-1) with changing the description. The response then doesn't maintain proper relation between the range parameters, instead these have changed to List from String (I feel, that is not the correct way).

shwstppr · 2021-09-09T09:18:27Z

@sureshanaparti I'm okay with whatever others decide or the correct way is but if the original issue doesn't make sense as you said then this PR should say that it fixes the issue.

blueorangutan · 2021-09-09T09:25:20Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 1193

sureshanaparti · 2021-09-09T09:35:53Z

@blueorangutan test

blueorangutan · 2021-09-09T09:37:02Z

@sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

resmo · 2021-09-09T10:11:03Z

LGTM, the api response is much clearer now and much easier to adjust on client side.

blueorangutan · 2021-09-09T19:21:21Z

Trillian test result (tid-1997)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33081 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5424-t1997-kvm-centos7.zip
Smoke tests completed. 89 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

yadvr · 2021-09-13T09:06:21Z

@sureshanaparti pl fix conflicts

yadvr · 2021-09-14T05:50:56Z

@sureshanaparti pl fix conflicts

nvazquez

Code LGTM, I'm for removing the deprecated fields on the API response but understand is kept for backward compatibility

…orsystemvms to ipranges (new parameter to hold the list of IP range details).

sureshanaparti · 2021-09-14T10:20:21Z

@blueorangutan package

blueorangutan · 2021-09-14T10:21:02Z

@sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-09-14T10:57:11Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 1241

sureshanaparti · 2021-09-14T12:06:02Z

@blueorangutan test

blueorangutan · 2021-09-14T12:08:02Z

@sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2021-09-14T23:30:25Z

Trillian test result (tid-2039)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 39205 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5424-t2039-kvm-centos7.zip
Smoke tests completed. 89 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

nvazquez · 2021-09-15T03:23:47Z

@shwstppr @rhtyd @weizhouapache can you review this one?

yadvr · 2021-09-15T05:21:54Z

Sure, I'll start but this may need some manual testing too.

DaanHoogland · 2021-09-15T08:57:03Z

Sure, I'll start but this may need some manual testing too.

why would this need manual testing @rhtyd , the response has been changed and can be verified in an automated test. some automated test can (and should) be updated.

yadvr · 2021-09-15T09:14:42Z

@DaanHoogland I saw no changes or introduction of a new marvin test and given the automated tests passed, this suggests there are no specific test that consume/rely on the API response - which is why I suggested somebody to manually test the UI/API.

yadvr · 2021-09-15T09:16:21Z

Nevermind my last comment, I saw Suresh's before and after API response change is additive - LGTM.

DaanHoogland · 2021-09-15T12:38:34Z

hm, my comment stands though. even trusting Suresh we need some to find how tests that use this and update them.

sureshanaparti requested review from nvazquez and shwstppr September 9, 2021 08:48

sureshanaparti added this to the 4.16.0.0 milestone Sep 9, 2021

shwstppr reviewed Sep 9, 2021

View reviewed changes

sureshanaparti mentioned this pull request Sep 9, 2021

API break in pod responses #2735

Closed

sureshanaparti changed the title ~~Updated pod response, moved the parameters: "startip, endip, vlanid, forsystemvms" to ipranges.~~ Updated pod response, grouped the parameters: "startip, endip, vlanid, forsystemvms" as ip range and added to ipranges parameter. Sep 9, 2021

nvazquez linked an issue Sep 13, 2021 that may be closed by this pull request

API break in pod responses #2735

Closed

nvazquez assigned sureshanaparti Sep 13, 2021

nvazquez added the component:api label Sep 13, 2021

nvazquez approved these changes Sep 14, 2021

View reviewed changes

Updated pod response, moved the parameters: startip, endip, vlanid, f…

487e0ad

…orsystemvms to ipranges (new parameter to hold the list of IP range details).

sureshanaparti force-pushed the pod-response-changes branch from 8a5e6e3 to 487e0ad Compare September 14, 2021 10:19

yadvr added the status:needs-testing label Sep 15, 2021

yadvr approved these changes Sep 15, 2021

View reviewed changes

yadvr merged commit 1f3f02b into apache:main Sep 15, 2021

DaanHoogland removed the status:needs-testing label Jan 28, 2026

Conversation

sureshanaparti commented Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

sureshanaparti commented Sep 9, 2021

Uh oh!

blueorangutan commented Sep 9, 2021

Uh oh!

shwstppr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sureshanaparti commented Sep 9, 2021

Uh oh!

sureshanaparti commented Sep 9, 2021

Uh oh!

shwstppr commented Sep 9, 2021

Uh oh!

blueorangutan commented Sep 9, 2021

Uh oh!

sureshanaparti commented Sep 9, 2021

Uh oh!

blueorangutan commented Sep 9, 2021

Uh oh!

resmo commented Sep 9, 2021

Uh oh!

blueorangutan commented Sep 9, 2021

Uh oh!

yadvr commented Sep 13, 2021

Uh oh!

yadvr commented Sep 14, 2021

Uh oh!

nvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

sureshanaparti commented Sep 14, 2021

Uh oh!

blueorangutan commented Sep 14, 2021

Uh oh!

blueorangutan commented Sep 14, 2021

Uh oh!

sureshanaparti commented Sep 14, 2021

Uh oh!

blueorangutan commented Sep 14, 2021

Uh oh!

blueorangutan commented Sep 14, 2021

Uh oh!

nvazquez commented Sep 15, 2021

Uh oh!

yadvr commented Sep 15, 2021

Uh oh!

DaanHoogland commented Sep 15, 2021

Uh oh!

yadvr commented Sep 15, 2021

Uh oh!

yadvr commented Sep 15, 2021

Uh oh!

DaanHoogland commented Sep 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

sureshanaparti commented Sep 9, 2021 •

edited

Loading

shwstppr left a comment •

edited

Loading